fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback by PierreBrisorgueil · Pull Request #3866 · pierreb-devkit/Node

PierreBrisorgueil · 2026-06-14T19:59:33Z

Summary

Critical pre-auth account-takeover removed: the OAuth callback contained a branch (req.body.strategy === false) that minted a TOKEN session cookie from a client-controlled identity without ever verifying a provider token. Any caller that posted a crafted strategy=false body with an arbitrary email could log in as any user with zero provider involvement.
Fix: the short-circuit branch is deleted; the callback now unconditionally falls through to Passport's authenticate(), which performs the actual provider-token verification before a session is issued.
Throttling: authLimiter (existing rate-limiter middleware) is applied to both the callback route and the OAuth initiation route, preventing brute-force enumeration of the former attack surface.
Regression test: a failing-first test covering the strategy=false body payload is included, asserting the callback rejects without a valid provider token.

Why

The removed branch was an unguarded escape hatch that let any HTTP client bypass the OAuth provider entirely and impersonate any account.

Scope

Module(s) impacted: modules/auth
Cross-module impact: none
Risk level: high

Validation

npm run lint
npm test
Manual checks done (if applicable)

Guardrails check

No secrets or credentials introduced (.env*, secrets/**, keys, tokens)
No risky rename/move of core stack paths
Changes remain merge-friendly for downstream projects
Tests added or updated when behavior changed

Notes for reviewers

Security considerations: this closes a critical pre-auth account-takeover. The removed strategy===false branch had no legitimate use — it was a vestigial fast-path that bypassed the provider entirely.
Mergeability considerations: no schema or API changes; backward-compatible.
Follow-up tasks: none for this PR.

Closes #3849
Part of the security audit epic #3848

Summary by CodeRabbit

Release Notes

Security Fixes
- OAuth authentication now exclusively relies on the OAuth provider as the source of authenticated identity, removing the previous fallback mechanism that accepted client-asserted identity.
Improvements
- Added rate limiting protection to OAuth strategy and callback endpoints.

…throttle callback

coderabbitai · 2026-06-14T19:59:41Z

Warning

Review limit reached

@PierreBrisorgueil, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 47 minutes and 26 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e30f25c7-9453-4e05-bf6e-0cb23e4a1315

📥 Commits

Reviewing files that changed from the base of the PR and between e4ee5d9 and d1ce8ec.

📒 Files selected for processing (2)

modules/auth/controllers/auth.controller.js
modules/auth/tests/auth.integration.tests.js

Walkthrough

Removes the req.body.strategy === false branch in oauthCallback that allowed unauthenticated clients to assert an OAuth identity via request body fields and receive a minted session token. All OAuth routes now enforce authLimiter, and the integration tests are updated with a security regression test covering the removed exploit path.

Changes

OAuth Callback Security Fix

Layer / File(s)	Summary
Remove client-asserted identity branch in `oauthCallback` `modules/auth/controllers/auth.controller.js`	Deletes the `req.body.strategy === false` code path (~37 lines) that accepted client-supplied `key`/`value` body fields to look up and mint a session token without any server-side provider verification. Identity is now exclusively derived from `passport.authenticate(...)`.
Apply `authLimiter` to OAuth routes `modules/auth/routes/auth.routes.js`	Adds `authLimiter` middleware to `GET /api/auth/:strategy` and both `GET`/`POST /api/auth/:strategy/callback` route definitions, which previously had no rate limiting.
Security regression test `modules/auth/tests/auth.integration.tests.js`	Removes the two tests that exercised the now-deleted client-side callback branch and replaces them with a test that seeds a victim user, submits a client-asserted identity POST, and asserts no `TOKEN` cookie is issued and the status is ≥ 400.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

pierreb-devkit/Node#3496: Modifies the same oauthCallback handler's handling of req.body-driven strategy/key logic, directly overlapping with the removed branch.
pierreb-devkit/Node#3268: Also changes how req.body.strategy/req.body.key is handled in oauthCallback, adding an allowlist guard for the same client-supplied field that this PR removes entirely.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the two main changes: removing the unverified client-asserted OAuth identity branch and adding rate-limiting to the callback.
Description check	✅ Passed	The description follows the template structure comprehensively, covering summary, scope, validation, guardrails, and security considerations with appropriate detail and verification checkmarks.
Linked Issues check	✅ Passed	All objectives from issue `#3849` are addressed: the unsafe `strategy === false` branch is removed from the controller, `authLimiter` is applied to both callback routes, and a regression test confirms rejection of crafted payloads.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the stated objectives: controller logic removal, route middleware application, and test coverage. No unrelated modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/oauth-callback-client-identity

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-06-14T20:01:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.48%. Comparing base (c31a5f0) to head (d1ce8ec).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3866   +/-   ##
=======================================
  Coverage   92.48%   92.48%           
=======================================
  Files         165      165           
  Lines        5400     5387   -13     
  Branches     1735     1730    -5     
=======================================
- Hits         4994     4982   -12     
+ Misses        326      325    -1     
  Partials       80       80

Flag	Coverage Δ
integration	`60.10% <100.00%> (-0.08%)`	⬇️
unit	`73.60% <0.00%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c31a5f0...d1ce8ec. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR hardens the OAuth flow in modules/auth by removing a client-controlled OAuth callback bypass that could mint a session without provider verification, and by applying existing rate-limiting to the OAuth initiation/callback routes.

Changes:

Removes the req.body.strategy === false OAuth callback branch that trusted client-asserted identity data.
Applies authLimiter to /api/auth/:strategy and /api/auth/:strategy/callback (GET + POST).
Replaces the prior “client-side OAuth callback” integration test with a regression test ensuring no TOKEN cookie is minted from a forged callback body.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
modules/auth/controllers/auth.controller.js	Deletes the insecure client-asserted identity branch and always delegates identity verification to Passport.
modules/auth/routes/auth.routes.js	Adds `authLimiter` to OAuth initiation and callback routes to reduce brute-force/enumeration risk.
modules/auth/tests/auth.integration.tests.js	Updates integration tests to cover the removed bypass with a security regression assertion.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modules/auth/tests/auth.integration.tests.js`:
- Around line 579-600: The UserService.create() call that creates the victim
user is currently outside the try/finally block, which means if an old user with
the same email exists from a previous test run, the create will fail with no
cleanup opportunity, causing flaky test failures. Move the victim user creation
inside the try block after its opening brace, and before creating the victim,
add a pre-cleanup step that attempts to remove any existing user with the
victimEmail (wrapped in a try-catch to ignore errors if the user doesn't exist).
This ensures the test starts with a clean state regardless of previous test
outcomes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: abc1e8e2-65db-47cd-809e-60061bd3466e

📥 Commits

Reviewing files that changed from the base of the PR and between c31a5f0 and e4ee5d9.

📒 Files selected for processing (3)

modules/auth/controllers/auth.controller.js
modules/auth/routes/auth.routes.js
modules/auth/tests/auth.integration.tests.js

… clarify comment - Move victim UserService.create() inside try/finally to avoid flakiness on reruns (pre-delete any leftover oauthcb-victim@test.com first) - Replace fragile `status >= 400` with `not.toBe(200)` + no-TOKEN-cookie; the failure path may 302-redirect rather than return 4xx, so the real security invariant (no session minted) is now the hard assertion - Reword oauthCallback comment: passport.authenticate() does verify the provider response; the unsafe part was trusting a client-asserted body identity with no provider-token check (that branch is now removed)

PierreBrisorgueil added 2 commits June 14, 2026 20:58

fix(auth): remove unverified client-asserted OAuth identity branch + …

39c2a03

…throttle callback

fix(auth): throttle the OAuth initiation route for consistency

e4ee5d9

PierreBrisorgueil added Fix A bug fix Severity5: access/security Bug qualification P1 Critical — must be done first security labels Jun 14, 2026

PierreBrisorgueil self-assigned this Jun 14, 2026

PierreBrisorgueil marked this pull request as ready for review June 14, 2026 20:01

Copilot AI review requested due to automatic review settings June 14, 2026 20:01

Copilot started reviewing on behalf of PierreBrisorgueil June 14, 2026 20:01 View session

Copilot AI reviewed Jun 14, 2026

View reviewed changes

Comment thread modules/auth/tests/auth.integration.tests.js Outdated

Comment thread modules/auth/controllers/auth.controller.js Outdated

coderabbitai Bot requested changes Jun 14, 2026

View reviewed changes

Comment thread modules/auth/tests/auth.integration.tests.js

coderabbitai Bot approved these changes Jun 14, 2026

View reviewed changes

PierreBrisorgueil merged commit c09de24 into master Jun 15, 2026
8 checks passed

PierreBrisorgueil deleted the fix/oauth-callback-client-identity branch June 15, 2026 06:46

PierreBrisorgueil mentioned this pull request Jun 15, 2026

🎯 Security audit hardening — Node + Vue (2026-06 audit) #3848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback#3866

fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback#3866
PierreBrisorgueil merged 3 commits into
masterfrom
fix/oauth-callback-client-identity

PierreBrisorgueil commented Jun 14, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 14, 2026 •

edited

Loading

Review limit reached

Uh oh!

codecov Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

PierreBrisorgueil commented Jun 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Scope

Validation

Guardrails check

Notes for reviewers

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

codecov Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PierreBrisorgueil commented Jun 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 14, 2026 •

edited

Loading

codecov Bot commented Jun 14, 2026 •

edited

Loading